feat: add client idle timeout [MCP-57] #383
Conversation
src/common/sessionStore.ts
Outdated
| import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; | ||
| import logger, { LogId, McpLogger } from "./logger.js"; | ||
|
|
||
| class TimeoutManager { |
There was a problem hiding this comment.
nit: can we add unit tests for this?
|
|
||
| export class SessionStore { | ||
| private sessions: { [sessionId: string]: StreamableHTTPServerTransport } = {}; | ||
| private sessions: { |
There was a problem hiding this comment.
[q] didn't check our telemetry PR but should we consider adding some http metrics? we could open a ticket for this, just checking about what you think. example: number of active sessions, timed out sessions
There was a problem hiding this comment.
does that add value? number of active sessions changes over time not sure how we would track it, we can add a time out session but bear in mind all of our telemetry is session based not server based, as it it lives inside of the MCP server not outside (http server).
There was a problem hiding this comment.
fair point - we can always check w/ analytics first hence the idea of a ticket
src/common/sessionStore.ts
Outdated
| import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; | ||
| import logger, { LogId, McpLogger } from "./logger.js"; | ||
|
|
||
| class TimeoutManager { |
There was a problem hiding this comment.
[q] I know is tiny but should we move this into it's own separate file?
There was a problem hiding this comment.
I didn't because this is not reused and it is private to the file, no strong opinion, can be moved
src/common/sessionStore.ts
Outdated
| import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; | ||
| import logger, { LogId, McpLogger } from "./logger.js"; | ||
|
|
||
| class TimeoutManager { |
There was a problem hiding this comment.
nit: can we add JS docs to the class and some of the methods below?
There was a problem hiding this comment.
almost none of our code has JS docs, we should add it across the board or not add any I think
Proposed changes
add client idle timeout [MCP-57]
Checklist